Allow PDF capture in headful mode. #375
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bensteinberg
approved these changes
Oct 30, 2024
|
(The Node 18 tests got hung up, so I canceled and re-ran them... looks good on this pass!) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
Our experiments, while limited, suggest that print-to-PDF works just fine with chromium in headful mode, even though Playwright's docs say, "Generating a pdf is currently only supported in Chromium headless", and their tests only check headless chromium
Digging through their codebase and git history, I was unable to uncover any information about why: there are no Github issues mentioning it being flaky/unreliable/buggy; there are no PRs adding "experimental support" or the like; the Chrome dev tools protocol doesn't mention any restrictions in its current verion OR in the oldest capture of that page in the Internet Archive...
I can see that the
pdfmethod on Playwright'sPageobject is optional and may beundefined, but I see no evidence that, if the method is available, that it is sometimes unreliable or untrusted (e.g., if running chromium in headful mode).And, indeed, there is a report from May 2020 where a user is complaining that
page.pdfis throwingTypeError: page.pdf is not a function; they closed the issue when they heard that headful mode was not supported.I think support for headful mode was added "by accident" due to some upstream change, and the docs and tests in the Playwright repo were never updated.
This PR
This PR removes the init check that disallows running Scoop with
--pdf-snapshopand--headless false.While it is certainly possible there is some good reason for not running that way, and further communication with the Playwright team will uncover why, it seems low-risk to enable for now... and perhaps collect our own data / watch for errors.